Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for i18n docs to lint:linkcheck script #361

Merged
merged 3 commits into from
Apr 22, 2022
Merged

Add support for i18n docs to lint:linkcheck script #361

merged 3 commits into from
Apr 22, 2022

Conversation

hippotastic
Copy link
Contributor

This fixes issue #103 by rewriting the broken link check scripts.

Because the previously used broken-link-checker package didn't work as expected for multi-language checks and the API docs did not match the actual behavior of the component, I rolled my own link checking logic based on the popular htmlparser2 library instead.

All link checks are now run locally and are based the output of the astro build command. The script parses the sitemap.xml in the build output directory, builds an index of all pages contained in the sitemap, checks for broken links (including invalid URL hashes) and outputs them in an easy-to-use format grouped by page.

@netlify
Copy link

netlify bot commented Apr 20, 2022

‼️ Deploy request for astro-docs-2 rejected.

Name Link
🔨 Latest commit 8e8d747

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic @hippotastic!

One thing: the docs repo uses pnpm so I think you need to remove package-lock.json and run pnpm i to update pnpm-lock.yaml.

Made a couple of suggestions below, but in general looks great!

scripts/lint-linkcheck.mjs Outdated Show resolved Hide resolved
scripts/lint-linkcheck.mjs Outdated Show resolved Hide resolved
@hippotastic
Copy link
Contributor Author

Thank you for the detailed review and great feedback! I updated my PR to address your valid suggestions.

Unfortunately, I found that none of the emojis get displayed correctly in Windows PowerShell and the old terminal, so I replaced them with plain text prefixes and added colors instead to allow faster scanning of the results. I also added a breakdown of the broken link types in the summary as you suggested.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! The plain text solution is actually way better than emojis.

How would you feel about using kleur instead of chalk for the terminal colours? It’s already in the transitive dependencies for the repo and is supposed to be smaller/faster than chalk (with a more or less interchangeable API I believe).

@hippotastic
Copy link
Contributor Author

Done. I usually prefer chalk because kleur is lacking bright colors (they have a long outstanding PR that would solve this, but don't seem to merge it due to confusion with bold, even though I can see a clear difference between bright and bold in my terminal).

However, as I now know that kleur is already being used by Astro, I'm happy to go with that, too.

@delucis
Copy link
Member

delucis commented Apr 21, 2022

Oh interesting! I'm on Mac and just saw bold rather than bright colours so I guess that explains the potential for confusion.

Anyway this looks great! Awesome work!

@hippotastic
Copy link
Contributor Author

@tony-sull Could you please have a look if this can be merged?

It's been reviewed by Chris already and I was informed to mention you now. Hope that's alright!

Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, this is excellent! Big fan of the color coded plaintext output, ran this locally and the output looks excellent and very clear 🥳

@tony-sull tony-sull merged commit ec14d76 into withastro:main Apr 22, 2022
@hippotastic hippotastic deleted the add-new-broken-link-checker branch April 22, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants